New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove one try-catch #3052
Remove one try-catch #3052
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3052 +/- ##
==========================================
- Coverage 88.7% 88.69% -0.01%
==========================================
Files 165 165
Lines 5718 5716 -2
Branches 1744 1744
==========================================
- Hits 5072 5070 -2
Misses 388 388
Partials 258 258
Continue to review full report at Codecov.
|
Thanks for the PR 🍺 It's super important to explain the motivation for your change. Please add a description to the pull request to explain why you made the change, what benefits it will have moving forward, and why it's better than the code that previously existed. |
@shellscape I don't quite understand your question. It's a simple refactoring, what's more do I have to explain? |
(I'm sure you know this, but for the sake of being verbose) Refactoring is typically for any number of reasons:
When you present a refactor, you should always explain and justify why the refactor is necessary. That's helpful to maintainers that are evaluating your change, and any onlookers that may be tracking the project's progress. It's also generally considered good form to thoroughly explain a pull request, and why we have a pull request template that encourages people to do so. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is a good change, considering we want to move to async-await anyway, and saves us three lines of code 😜
@lukastaegert for whatever reason, introducing this change is causing 4 of the tests to fail. |
I fear we have some blinking tests here, which is unrelated to this PR. I guess I have to look into this first before we merge any PRs. I think there is a race condition that can lead to undeterministic chunking in one test. |
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
I changed the code from this:
to this: